Skip to content

Add deletion policy field and decision logic - #32

Merged
nmechack27 merged 9 commits into
mainfrom
Mechack/DeleteNamespace
Oct 29, 2026
Merged

nmechack27 merged 9 commits into
mainfrom
Mechack/DeleteNamespace

Conversation

@nmechack27

@nmechack27 nmechack27 commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

What was changed

Adds the deletion policy field and the decision logic for deleting a Temporal Cloud namespace.

  • pkg/deletion: shared logic for any Temporal Cloud resource: resolving the policy, Preflight (decisions that need no Cloud call: Retain, never created).
  • controller/namespace/delete.go: the namespace rules (Observed, Decide), matching Temporal Cloud's delete behavior. Only an Active namespace can be deleted; delete protection, Nexus endpoints, and suspension block it; a namespace whose creation time doesn't match the one the operator recorded is never deleted.

Issue #9

@CLAassistant

CLAassistant commented Oct 25, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@nmechack27

Copy link
Copy Markdown
Collaborator Author

@jaypipes @tomba7 @zainawaisn

  1. What should happen on delete when spec.deletionPolicy is not set?
    I'm thinking if the resource was created by the operator, delete and if it was adopted retain.

  2. Before sending the delete request, Decide checks for delete protection and Nexus endpoints itself, even though Temporal Cloud already rejects the delete in both cases. Should I keep those checks, or just send the delete and surface Cloud's error? Checking first gives clearer messages and avoids repeated rejected deletes.
    Relying on Cloud means less duplicated logic but its error codes overlap (in both cases temporal returns FailedPrecondition), so we'd have to show its message as is.

@tomba7

tomba7 commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

What should happen on delete when spec.deletionPolicy is not set?
I'm thinking if the resource was created by the operator, delete and if it was adopted retain.

IMO we should definitely set a default. My hunch is that most users who are familiar with ACK/Crossplane will expect Delete, since that's the default in those tools. But apart from the familiarity, either option is fine with me. I'd say we go with Delete by default (regardless of how it was created) and then adjust it once we have customer feedback or when we get to the resource adoption implementation.

Before sending the delete request, Decide checks for delete protection and Nexus endpoints itself, even though Temporal Cloud already rejects the delete in both cases. Should I keep those checks, or just send the delete and surface Cloud's error? Checking first gives clearer messages and avoids repeated rejected deletes.
Relying on Cloud means less duplicated logic but its error codes overlap (in both cases temporal returns FailedPrecondition), so we'd have to show its message as is.

On the surface, the delete protection check seems like an obvious one to keep but unsure about the Nexus endpoint one since I haven't reviewed the PR just yet (will do shortly). In general, my preference is to keep the operator's own checks to a minimum, make tcloud the source of truth, and only add operator side checks when it's obvious or necessary (e..g the namespace CRD only has basic validation and leaves the rest to tcloud)

Comment thread api/cloud/common/deletion_policy.go Resolved
Comment thread api/cloud/v1alpha1/namespace.go Resolved
Comment thread api/cloud/v1alpha1/namespace.go Resolved
Comment thread api/cloud/v1alpha1/namespace.go Resolved
Comment thread pkg/deletion/deletion.go Resolved
@nmechack27
nmechack27 force-pushed the Mechack/DeleteNamespace branch from bda7408 to 1473a1d Compare October 27, 2026 07:30
@nmechack27
nmechack27 requested a review from jaypipes October 27, 2026 07:31

@jaypipes jaypipes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getting closer @niyomukiza-mechack! :)

Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread api/cloud/common/const.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
@nmechack27
nmechack27 force-pushed the Mechack/DeleteNamespace branch from 1473a1d to 63d97ba Compare October 28, 2026 17:04
@nmechack27
nmechack27 requested review from jaypipes and tomba7 October 28, 2026 17:21
Comment thread controller/namespace/delete.go Resolved
Comment thread api/cloud/common/const.go Resolved
Comment thread controller/namespace/delete.go Resolved
Comment thread controller/namespace/delete.go Resolved
@nmechack27
nmechack27 requested a review from jaypipes October 29, 2026 17:44

@tomba7 tomba7 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @niyomukiza-mechack

@nmechack27
nmechack27 merged commit b71eec4 into main Oct 29, 2026
7 checks passed
@nmechack27
nmechack27 deleted the Mechack/DeleteNamespace branch October 29, 2026 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants